Greptile Overview

Greptile Summary

This PR migrates IsaacLab from Omniverse's omni.log to Python's standard logging module across 59 files. The changes include:

• New logging infrastructure in simulation_context.py and utils.py with ColoredFormatter for console output and RateLimitFilter to throttle duplicate warnings
• Configuration options added to SimulationCfg: logging_level (default: WARNING) and save_logs_to_file (default: True)
• Consistent migration pattern: All files replace omni.log.warn() → logger.warning(), omni.log.verbose() → logger.debug(), omni.log.error() → logger.error(), omni.log.info() → logger.info()
• Logs saved to temp directory with timestamp, format: isaaclab_YYYY-MM-DD_HH-MM-SS.log

Key issue identified: The _setup_logger() method unconditionally adds handlers to the root logger without checking if they already exist, which could cause duplicate log messages if SimulationContext is re-instantiated or if other libraries also configure the root logger.

Confidence Score: 3/5

• This PR is mostly safe to merge but requires addressing the duplicate handler issue to prevent log message duplication
• The migration from omni.log to Python logging is implemented consistently across all 59 files with proper mappings (verbose→debug, warn→warning). However, the _setup_logger() method has a critical flaw where it unconditionally adds handlers to the root logger without checking for existing handlers. This could cause duplicate log messages if SimulationContext is re-instantiated or if other code also configures the root logger. The core functionality is sound, but this issue should be fixed before merging to avoid production logging problems.
• source/isaaclab/isaaclab/sim/simulation_context.py - Fix duplicate handler issue in _setup_logger()

Important Files Changed

File Analysis

Sequence Diagram

sequenceDiagram
    participant User
    participant SimContext as SimulationContext
    participant Logger as Root Logger
    participant Console as Console Handler
    participant File as File Handler
    participant Module as Module Code

    User->>SimContext: __init__(cfg)
    SimContext->>SimContext: _setup_logger()
    SimContext->>Logger: getLogger() [root]
    SimContext->>Logger: setLevel(cfg.logging_level)
    SimContext->>Console: Create StreamHandler(sys.stdout)
    SimContext->>Console: Add ColoredFormatter
    SimContext->>Console: Add RateLimitFilter(5s)
    SimContext->>Logger: addHandler(Console)
    
    alt save_logs_to_file=True
        SimContext->>File: Create FileHandler(temp_dir)
        SimContext->>File: Add Formatter with line numbers
        SimContext->>Logger: addHandler(File)
    end
    
    SimContext-->>SimContext: return root_logger
    
    Note over Module: During execution
    Module->>Logger: getLogger(__name__)
    Module->>Logger: logger.warning("message")
    Logger->>Console: Check RateLimitFilter
    Console->>Console: Rate limit WARNING messages
    Logger->>Console: Format with ColoredFormatter
    Console->>User: Display colored log
    Logger->>File: Write to temp file (DEBUG level)

_{59 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

Greptile Overview

Greptile Summary

This PR migrates IsaacLab from omni.log to Python's standard logging module, implementing custom formatters and filters for colored output and rate limiting.

Key changes:

• Replaced all omni.log.warn() calls with self.logger.warning() in SimulationContext
• Added _setup_logger() method that configures the root logger with ColoredFormatter and RateLimitFilter
• Implemented optional file logging to temp directory
• Added logging level configuration via SimulationCfg.logging_level

Critical issues found:

1. Missing import: datetime is used on line 1012 but never imported, causing immediate runtime failure when file logging is enabled
2. Handler management: Unconditionally removes ALL handlers from root logger, breaking logging from other libraries and components
3. Resource leak: File handlers are never closed, causing file handle leaks on repeated instantiation
4. Global rate limiting: RateLimitFilter applies to ALL WARNING messages system-wide, potentially hiding critical warnings from third-party libraries
5. Duplicate handler issue: As noted in previous comments, re-instantiating SimulationContext will keep adding handlers despite the removal logic

Confidence Score: 1/5

• This PR has critical bugs that will cause runtime failures and resource leaks
• The missing datetime import on line 1012 will cause an immediate NameError when file logging is enabled (which is the default per simulation_cfg.py:417). Additionally, the unconditional removal of all root logger handlers and the lack of file handler cleanup create serious issues for production use
• source/isaaclab/isaaclab/sim/simulation_context.py requires immediate fixes before merge

Important Files Changed

File Analysis

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant SC as SimulationContext
    participant Logger as Root Logger
    participant Handler as StreamHandler/FileHandler
    participant CF as ColoredFormatter
    participant RLF as RateLimitFilter
    participant File as Log File

    App->>SC: __init__()
    SC->>SC: _setup_logger()
    SC->>Logger: logging.getLogger()
    Logger-->>SC: root_logger
    SC->>Logger: setLevel(cfg.logging_level)
    
    Note over SC,Logger: Critical: Removes ALL existing handlers
    SC->>Logger: removeHandler() for each handler
    
    SC->>Handler: Create StreamHandler(sys.stdout)
    SC->>CF: ColoredFormatter()
    SC->>RLF: RateLimitFilter(interval_seconds=5)
    SC->>Handler: setFormatter(ColoredFormatter)
    SC->>Handler: addFilter(RateLimitFilter)
    SC->>Logger: addHandler(StreamHandler)
    
    alt cfg.save_logs_to_file
        Note over SC,File: Missing datetime import causes error here
        SC->>File: Create temp file path
        SC->>Handler: Create FileHandler(log_file_path)
        SC->>Handler: setFormatter(file_formatter)
        SC->>Logger: addHandler(FileHandler)
        Note over Handler,File: File handle never closed (leak)
    end
    
    SC-->>App: logger instance
    
    Note over App,RLF: Runtime: WARNING messages rate limited globally
    App->>Logger: logger.warning("message")
    Logger->>RLF: filter(record)
    RLF-->>Logger: True/False (rate limited)
    alt Not rate limited
        Logger->>Handler: emit(record)
        Handler->>CF: format(record)
        CF-->>Handler: colored message
        Handler->>File: write to stdout/file
    end

_{1 file reviewed, 4 comments}

_{Edit Code Review Agent Settings | Greptile}

Greptile Overview

Greptile Summary

This PR migrates IsaacLab from Omniverse's omni.log to Python's standard logging module. The migration is comprehensive, touching 59 files across the codebase.

Key Changes:

• Introduced ColoredFormatter and RateLimitFilter classes in sim/utils.py for custom log formatting and warning rate limiting
• Added logging configuration to SimulationCfg: logging_level (default: "WARNING") and save_logs_to_file (default: True)
• Centralized logger setup in SimulationContext._setup_logger() that configures the root logger
• Replaced all omni.log calls with standard Python logging.getLogger(__name__) pattern across all modules

Issues Identified:
The migration has several design issues flagged in previous comments that need addressing:

1. Handler management: Removes ALL existing handlers from root logger unconditionally, which could break logging from other components (simulation_context.py:991)
2. Duplicate handlers: No check for existing handlers before adding new ones, causing duplicate log messages if SimulationContext is reinstantiated (simulation_context.py:991)
3. Missing import: datetime is used but import is missing (flagged in previous comment, should use time.strftime instead)
4. File handle leaks: File handler is never closed if SimulationContext is instantiated multiple times (simulation_context.py:1014)
5. Global rate limiting: Rate limiter applies to ALL WARNING messages from any module, potentially suppressing critical warnings from third-party libraries

Positive Aspects:

• The conversion from omni.log to Python logging is consistent across all files
• Log format is clear and includes timestamps, filenames, and log levels
• File logging to temp directory provides helpful debugging trail
• Pattern of logger = logging.getLogger(__name__) follows Python best practices

Confidence Score: 3/5

• This PR has multiple logical issues in the logging setup that could cause duplicate logs, handler leaks, and suppressed warnings in production
• Score reflects a well-intentioned migration with consistent code changes across 59 files, but the core logging infrastructure has several critical design flaws. The issues with handler management (removing all handlers unconditionally, no duplicate checks, file handle leaks) and global rate limiting could cause problems in production environments, especially when SimulationContext is reinstantiated or when running alongside other Python code. The missing datetime import would cause immediate runtime errors. These are fixable issues but require careful attention before merge.
• Primary attention needed on source/isaaclab/isaaclab/sim/simulation_context.py for the _setup_logger() method (lines 991-1025). The logging configuration needs to handle existing handlers properly, close file handles, and avoid global side effects.

Important Files Changed

File Analysis

Sequence Diagram

sequenceDiagram
    participant App as Application
    participant SimCfg as SimulationCfg
    participant SimCtx as SimulationContext
    participant Logger as Root Logger
    participant Handler as StreamHandler
    participant FileHandler as FileHandler
    participant Filter as RateLimitFilter

    App->>SimCfg: Create config with logging_level
    App->>SimCtx: __init__(cfg)
    SimCtx->>SimCtx: _setup_logger()
    SimCtx->>Logger: getLogger()
    SimCtx->>Logger: setLevel(cfg.logging_level)
    
    Note over SimCtx,Logger: Issue: Removes ALL existing handlers
    SimCtx->>Logger: removeHandler(all handlers)
    
    SimCtx->>Handler: Create StreamHandler(sys.stdout)
    SimCtx->>Handler: setFormatter(ColoredFormatter)
    SimCtx->>Filter: Create RateLimitFilter(5s)
    Note over Filter: Applies globally to ALL WARNING messages
    SimCtx->>Handler: addFilter(RateLimitFilter)
    SimCtx->>Logger: addHandler(StreamHandler)
    
    alt save_logs_to_file == True
        SimCtx->>FileHandler: Create FileHandler(temp_file)
        Note over FileHandler: Issue: Never closed, leaks handles
        SimCtx->>Logger: addHandler(FileHandler)
    end
    
    App->>App: Use logger throughout lifecycle
    Note over Logger: All modules share root logger configuration

_{59 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

Greptile Overview

Greptile Summary

This PR migrates IsaacLab from Omniverse's proprietary omni.log logging system to Python's standard logging module. The change affects only one file in the current changeset (controllers/utils.py), but is part of a broader migration across the entire codebase.

Key Changes in controllers/utils.py:

• Replaced import nvidia.srl.tools.logger and import omni.log with import logging
• Changed logger initialization from external logger to logging.getLogger(__name__)
• Migrated omni.log.warn() calls to logger.warning()
• Updated log_level parameter from logger.level_from_name("ERROR") to logging.ERROR

Issues Found:

• Incorrect log levels: Verbose informational output (lines 95-97, 132-134) uses logger.warning() when it should use logger.info() or logger.debug(). This will cause the RateLimitFilter to rate-limit normal verbose output.
• Error misclassification: Line 99 logs an actual error condition with logger.warning() instead of logger.error()

These issues were pre-existing in the old code (using omni.log.warn()) and have been carried over to the new logging system, but should be corrected for proper log level semantics.

Confidence Score: 4/5

• This PR is safe to merge with minor log level issues that should be addressed
• The migration from omni.log to Python's logging is implemented correctly with proper module-level logger initialization and appropriate constant usage. However, the score is reduced from 5 to 4 due to incorrect log level usage: verbose informational messages are logged as warnings (triggering rate limiting), and an actual error condition is logged as a warning instead of an error. These issues don't affect functionality but reduce logging effectiveness.
• Pay attention to source/isaaclab/isaaclab/controllers/utils.py - the log level issues should be corrected for proper logging semantics

Important Files Changed

File Analysis

Sequence Diagram

sequenceDiagram
    participant User
    participant SimulationContext
    participant RootLogger
    participant StreamHandler
    participant FileHandler
    participant ColoredFormatter
    participant RateLimitFilter
    participant ModuleLogger as Module Logger (utils.py)
    
    User->>SimulationContext: __init__(cfg)
    SimulationContext->>SimulationContext: _setup_logger()
    SimulationContext->>RootLogger: getLogger()
    SimulationContext->>RootLogger: setLevel(cfg.logging_level)
    SimulationContext->>RootLogger: removeHandler() (all existing)
    
    SimulationContext->>StreamHandler: create (stdout)
    SimulationContext->>ColoredFormatter: create formatter
    SimulationContext->>RateLimitFilter: create filter (5s interval)
    StreamHandler->>ColoredFormatter: setFormatter()
    StreamHandler->>RateLimitFilter: addFilter()
    SimulationContext->>RootLogger: addHandler(StreamHandler)
    
    alt cfg.save_logs_to_file == True
        SimulationContext->>FileHandler: create (temp file)
        SimulationContext->>FileHandler: setFormatter()
        SimulationContext->>RootLogger: addHandler(FileHandler)
    end
    
    Note over ModuleLogger: Module creates logger instance
    ModuleLogger->>RootLogger: getLogger(__name__)
    
    ModuleLogger->>ModuleLogger: logger.warning("message")
    ModuleLogger->>RootLogger: propagate to root
    RootLogger->>RateLimitFilter: filter(record)
    RateLimitFilter-->>RootLogger: True/False (rate limited)
    RootLogger->>StreamHandler: handle(record)
    StreamHandler->>ColoredFormatter: format(record)
    ColoredFormatter-->>StreamHandler: colored output
    StreamHandler->>User: print to stdout
    
    opt if FileHandler exists
        RootLogger->>FileHandler: handle(record)
        FileHandler->>User: write to log file
    end

_{1 file reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}